Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

src/lib/services/firebase/oauth/types.ts (1)

12-18: Redundant field declarations in extended interface.

OAuthTokens extends Credentials from google-auth-library, which already declares access_token, refresh_token, token_type, and expiry_date. Re-declaring them here is redundant but documents the expected shape explicitly. Consider removing the duplicates or adding a comment explaining the intent.

src/lib/services/firebase/api/types.ts (1)

31-35: Consider adding 'STATE_UNSPECIFIED' to project state union.

The Firebase Management API can return STATE_UNSPECIFIED in addition to ACTIVE and DELETED. Adding it would make the type more robust against unexpected API responses.

Suggested change

   /**
    * Project state.
    */
-  state: 'ACTIVE' | 'DELETED';
+  state: 'STATE_UNSPECIFIED' | 'ACTIVE' | 'DELETED';

src/ui/components/FirebaseStatusDisplay.tsx (3)

99-109: Potential non-unique React key if errors have duplicate path+message.

The key ${error.path}-${error.message} could collide if the same error appears multiple times. Consider appending the index for guaranteed uniqueness.

Suggested fix

-              {credential.errors.slice(0, 3).map((error) => (
-                <Text key={`${error.path}-${error.message}`} color="red">
+              {credential.errors.slice(0, 3).map((error, index) => (
+                <Text key={`${error.path}-${error.message}-${index}`} color="red">

---

154-166: Consider extracting platform-needs helpers to a shared utility.

platformNeedsAndroid and platformNeedsIos duplicate logic from downloader.ts (lines 179-181). Extract to a shared utility in the firebase module to maintain consistency.

---

180-187: getStatusColor returns 'red' for both missing and n/a statuses.

Currently n/a and ✗ both render as red. Consider using a neutral color (e.g., dimColor or gray) for n/a to differentiate "not applicable" from "missing/error".

Suggested fix

 function getStatusColor(status: string): string {
   if (status === '✓') return 'green';
   if (status === '!') return 'yellow';
+  if (status === 'n/a') return 'gray';
   return 'red';
 }

src/lib/services/firebase/downloader.ts (1)

145-168: Consider adding error handling for file write operations.

downloadAndroidConfig and downloadIosConfig don't catch errors from fs.mkdir or fs.writeFile. If the directory creation or file write fails (e.g., permission denied), the error propagates uncaught. Consider wrapping in try-catch with meaningful error messages.

src/ui/components/FirebaseWizard.tsx (1)

656-706: Consider simplifying callback dependency management.

The ref-based pattern to avoid circular dependencies works but adds complexity. Consider extracting the download flow into a custom hook or using a reducer pattern for cleaner state management.

src/lib/services/firebase/firebase-service.ts (1)

64-71: Consider extracting platform requirements logic to a shared utility.

The needsAndroid/needsIos computation is duplicated across multiple files (here, in FirebaseWizard.tsx, and in detector.ts). Extract this to a shared helper function to ensure consistency.

♻️ Example shared utility

// In types.ts or a new utils.ts
export function getPlatformRequirements(platform: Platform): { needsAndroid: boolean; needsIos: boolean } {
  const needsAndroid = platform === 'android' || platform === 'react-native' || platform === 'flutter';
  const needsIos = platform === 'ios' || platform === 'react-native' || platform === 'flutter';
  return { needsAndroid, needsIos };
}

src/lib/services/firebase/api/firebase-api.ts (2)

39-56: Consider adding request timeout.

The fetch calls have no timeout configuration. Long-running or hung requests could block the CLI indefinitely. Consider using AbortController with a timeout.

♻️ Example with timeout

 private async request<T>(path: string): Promise<T> {
   const token = await this.getAccessToken();
   const url = `${BASE_URL}${path}`;
+  const controller = new AbortController();
+  const timeoutId = setTimeout(() => controller.abort(), 30000);

-  const response = await fetch(url, {
-    headers: {
-      Authorization: `Bearer ${token}`,
-      'Content-Type': 'application/json',
-    },
-  });
+  try {
+    const response = await fetch(url, {
+      headers: {
+        Authorization: `Bearer ${token}`,
+        'Content-Type': 'application/json',
+      },
+      signal: controller.signal,
+    });

     if (!response.ok) {
       const error = await response.text();
       throw new Error(`Firebase API error (${response.status}): ${error}`);
     }

     return response.json() as Promise<T>;
+  } finally {
+    clearTimeout(timeoutId);
+  }
 }

---

103-106: Consider URL-encoding path parameters defensively.

While Firebase project IDs and app IDs typically don't contain special characters, encoding them with encodeURIComponent would be a defensive measure against edge cases.

src/lib/services/firebase/types.ts (2)

86-90: Naming conflict with existing ValidationError class.

This ValidationError interface shadows the ValidationError class in src/lib/errors/types.ts. Consider renaming to FirebaseValidationError or CredentialValidationError to avoid confusion when both are imported.

---

104-125: Consider documenting the valid/content invariant.

The content field is optional, but when valid is true, content should always be defined. This invariant is relied upon in firebase-service.ts. Consider adding a JSDoc comment to document this relationship.

📝 Documentation suggestion

 export interface FirebaseCredentialFile {
   /** File path relative to project root */
   path: string;
   /** Absolute file path */
   absolutePath: string;
   /** Target platform */
   platform: 'android' | 'ios';
   /** Credential file type */
   type: CredentialFileType;
   /** Whether file exists */
   exists: boolean;
   /** Whether file is valid */
   valid: boolean;
   /** Validation errors if any */
   errors: ValidationError[];
-  /** Parsed content if valid */
+  /** Parsed content if valid. Guaranteed to be defined when `valid` is true. */
   content?: GoogleServicesJson | GoogleServiceInfoPlist;
   /** Whether file is in expected location */
   inExpectedLocation: boolean;
   /** Expected location if different from actual */
   expectedPath?: string;
 }

src/lib/services/firebase/detector.ts (1)

607-623: Reuse the existing needsAndroidConfig/needsIosConfig helpers.

Avoids duplicating platform logic and keeps behavior consistent.

♻️ Suggested refactor

-  const needsAndroid =
-    platform === 'android' || platform === 'react-native' || platform === 'flutter';
-  const needsIos = platform === 'ios' || platform === 'react-native' || platform === 'flutter';
+  const needsAndroid = needsAndroidConfig(platform);
+  const needsIos = needsIosConfig(platform);

src/lib/services/firebase/oauth/token-store.ts (1)

45-53: Ensure existing token files are re-permissioned too.

fs.writeFile(..., { mode }) only sets permissions on newly created files; if a token file already exists from an older version, it can keep permissive bits. Consider chmod after write (and optionally on the directory) to guarantee owner-only access on upgrades.

🔐 Suggested hardening

   await fs.mkdir(dir, { recursive: true, mode: 0o700 });
   // Write token file with restricted permissions (owner read/write only)
   await fs.writeFile(this.tokenPath, JSON.stringify(tokens, null, 2), {
     encoding: 'utf-8',
     mode: 0o600,
   });
+  // Ensure existing paths are tightened as well
+  await fs.chmod(dir, 0o700);
+  await fs.chmod(this.tokenPath, 0o600);

src/ui/components/FirebaseWizard.tsx (1)

150-169: Handle spawn errors to avoid CLI crashes when browser open fails.

If open/xdg-open/cmd isn’t available, spawn emits an error event; without a handler, Node can throw. Add an error listener (and ideally surface a user-facing message).

🛠 Minimal safeguard

-  spawn(command, args, { detached: true, stdio: 'ignore' }).unref();
+  const child = spawn(command, args, { detached: true, stdio: 'ignore' });
+  child.on('error', (err) => {
+    // eslint-disable-next-line no-console
+    console.error('Failed to open browser:', err.message);
+  });
+  child.unref();

llms.txt (1)

586-617: Document the OAuth download functionality.

The documentation describes detection and validation features well, but it doesn't mention a key capability from the PR objectives: the ability to download missing Firebase configurations from the Firebase Console using OAuth 2.0 with PKCE. The example shows "Press Enter to configure" but doesn't explain what this action does.

📝 Suggested enhancement to document OAuth download feature

Consider adding a bullet point under Features: to mention the download capability:

 **Features:**
 - Auto-detects Firebase credential files in project
 - Validates JSON/plist structure against Firebase schema
 - Reports issues (missing files, invalid format, wrong location)
 - Provides recommendations with help URLs
+- Downloads missing configs from Firebase Console via OAuth 2.0
 - Supports React Native, Flutter, iOS, and Android projects

And clarify the interactive action in the example section:

-Press Enter to configure, Esc to skip
+Press Enter to download missing configs (opens browser for Firebase OAuth), Esc to skip

Based on PR objectives, the OAuth download feature is a significant capability that users should be aware of.

src/lib/services/firebase/detector.ts (1)

632-655: unknown platform can still mark dual-native as configured with only one config (and suppress issues).

When detectPlatform() returns 'unknown' for dual-native repos, configured becomes true if either config is valid. That contradicts “fully configured” semantics and hides missing-config issues. Consider deriving needsAndroid/needsIos from detectNativePlatforms() when platform is 'unknown', and use those flags for both issue generation and configured.

🛠️ Proposed fix

 export async function detectFirebaseConfig(projectPath: string): Promise<FirebaseDetectionResult> {
   const platform = await detectPlatform(projectPath);
+  const native =
+    platform === 'unknown' ? await detectNativePlatforms(projectPath) : { hasIos: false, hasAndroid: false };
   const expectedPaths = getExpectedPaths(platform, projectPath);

   const android = await detectAndroidCredential(projectPath, expectedPaths.android);
   const ios = await detectIosCredential(projectPath, expectedPaths.ios);

-  const issues = generateIssues(android, ios, platform, expectedPaths);
+  const needsAndroid =
+    needsAndroidConfig(platform) || (platform === 'unknown' && native.hasAndroid);
+  const needsIos =
+    needsIosConfig(platform) || (platform === 'unknown' && native.hasIos);
+  const issues =
+    platform === 'unknown'
+      ? [
+          ...(needsAndroid ? generateAndroidIssues(android, expectedPaths.android[0]) : []),
+          ...(needsIos ? generateIosIssues(ios, expectedPaths.ios[0]) : []),
+        ]
+      : generateIssues(android, ios, platform, expectedPaths);

   // Determine if Firebase is configured
-  const needsAndroid =
-    platform === 'android' || platform === 'react-native' || platform === 'flutter';
-  const needsIos = platform === 'ios' || platform === 'react-native' || platform === 'flutter';
-
   let configured: boolean;
   if (platform === 'unknown') {
-    // For unknown platform, configured is true only if at least one valid config exists
-    configured = (android?.valid ?? false) || (ios?.valid ?? false);
+    const androidConfigured = !needsAndroid || (android?.valid ?? false);
+    const iosConfigured = !needsIos || (ios?.valid ?? false);
+    configured = androidConfigured && iosConfigured;
   } else {
     const androidConfigured = !needsAndroid || (android?.valid ?? false);
     const iosConfigured = !needsIos || (ios?.valid ?? false);
     configured = androidConfigured && iosConfigured;
   }

src/lib/services/firebase/detector.ts (1)

634-667: unknown platform can mask missing configs in dual-native repos.

When detectPlatform returns unknown for dual-native projects, configured becomes true if either config is valid and issues stay empty (because needsAndroidConfig/needsIosConfig ignore unknown). This can hide a missing iOS or Android config. Consider using detectNativePlatforms to determine which platforms exist and generate issues/configured accordingly.

🛠️ Suggested fix (keeps current behavior for truly unknown projects)

 export async function detectFirebaseConfig(projectPath: string): Promise<FirebaseDetectionResult> {
   const platform = await detectPlatform(projectPath);
   const expectedPaths = getExpectedPaths(platform, projectPath);

   const android = await detectAndroidCredential(projectPath, expectedPaths.android);
   const ios = await detectIosCredential(projectPath, expectedPaths.ios);

-  const issues = generateIssues(android, ios, platform, expectedPaths);
+  let issues = generateIssues(android, ios, platform, expectedPaths);

   // Determine if Firebase is configured
-  // For unknown platform, check if at least one valid config file exists
-  const needsAndroid =
-    platform === 'android' || platform === 'react-native' || platform === 'flutter';
-  const needsIos = platform === 'ios' || platform === 'react-native' || platform === 'flutter';
+  let needsAndroid =
+    platform === 'android' || platform === 'react-native' || platform === 'flutter';
+  let needsIos = platform === 'ios' || platform === 'react-native' || platform === 'flutter';

   let configured: boolean;
   if (platform === 'unknown') {
-    // For unknown platform, configured is true only if at least one valid config exists
-    configured = (android?.valid ?? false) || (ios?.valid ?? false);
+    const native = await detectNativePlatforms(projectPath);
+    if (native.hasAndroid || native.hasIos) {
+      needsAndroid = native.hasAndroid;
+      needsIos = native.hasIos;
+      issues = [];
+      if (needsAndroid) {
+        issues.push(...generateAndroidIssues(android, expectedPaths.android[0]));
+      }
+      if (needsIos) {
+        issues.push(...generateIosIssues(ios, expectedPaths.ios[0]));
+      }
+      const androidConfigured = !needsAndroid || (android?.valid ?? false);
+      const iosConfigured = !needsIos || (ios?.valid ?? false);
+      configured = androidConfigured && iosConfigured;
+    } else {
+      configured = (android?.valid ?? false) || (ios?.valid ?? false);
+    }
   } else {
     const androidConfigured = !needsAndroid || (android?.valid ?? false);
     const iosConfigured = !needsIos || (ios?.valid ?? false);
     configured = androidConfigured && iosConfigured;
   }

src/lib/services/firebase/detector.ts (1)

184-210: Expand iOS expected paths for RN/native layouts.

For react-native and ios, expectedPaths.ios only includes ios/GoogleService-Info.plist or root. Common layouts place the plist under ios/<App>/ or <App>/ (sibling to .xcodeproj), which will be flagged as “misplaced” and produce misleading recommendations. Consider deriving iOS expected paths from detected app directories (e.g., scan for .xcodeproj/.xcworkspace siblings or reuse the app-specific plist discovery results) and include those paths in expectedPaths.ios.